Add codec to encode/decode GRPC data frame. by fengli79 · Pull Request #414 · envoyproxy/envoy

fengli79 · 2017-02-02T22:23:47Z

Add codec to encode/decode GRPC data frame.

This reverts commit 4f52eb5.

PiotrSikora · 2017-02-03T14:19:33Z

Is there any reason why this is written from scratch and not using https://github.com/grpc/grpc?

mattklein123 · 2017-02-03T16:18:58Z

Per @PiotrSikora it would be very, very, very nice if we could work with the gRPC c++ folks to refactor slightly so that we can consume the generated RPC stubs (codec, etc.) but provide our own transport. I already replicated some stuff for unary calls out of expediency, but now that you are all involved it would be great to fix this for real. Can we at least SWAG what it would take before reimplementing?

louiscryan · 2017-02-03T18:35:02Z

@ctiller - Hey Craig! Its probably a good time to start discussing some of the integration points between Envoy and GRPC

In this particular instance WDYT? Would it be easy to factor out the framing code from core? If not then this should probably proceed as is and we have a longer term goal around factoring

fengli79 · 2017-02-08T00:32:28Z

Any chance to get this submitted? Refactor GRPC c core to reuse the framer is not trivial and won't happen any time soon.
This pull request is to unblock the data path for grpc-web. For the control plane GRPC requests initiated by envoy can be a different topic.

Here's the situation:

for grpc-web, we need a generic client, means the target grpc method is denoted by the request on the wire. Thus no code gen needed.
for grpc request to control plane services, code gen might be helpful, but not a blocker, if code gen is highly desired, you can start to use it anyway, remained part is merely optimization.

mattklein123 · 2017-02-08T00:57:52Z

@fengli79 I wasn't aware this was ready for review. @RomanDzhabarov can you take a first pass?

RomanDzhabarov · 2017-02-08T05:32:29Z

will check this tonight

RomanDzhabarov · 2017-02-08T06:24:31Z

source/common/grpc/codec.h

+  // @param input supplies the binary octets wrapped in a GRPC data frame.
+  // @param input supplies the buffer to store the decoded data.
+  // @return bool whether the decoding success.
+  bool Decode(Buffer::Instance& input, std::vector<Frame>* output);


usually in envoy we pass by ref in this case (less to deal with null checks, alloc/dealloc/etc).

RomanDzhabarov · 2017-02-08T06:24:41Z

source/common/grpc/codec.h

+
+  // Decodes the given buffer with GRPC data frame.
+  // @param input supplies the binary octets wrapped in a GRPC data frame.
+  // @param input supplies the buffer to store the decoded data.


fix param name

RomanDzhabarov · 2017-02-08T06:34:05Z

source/common/grpc/codec.h

+  // @param flags supplies the GRPC data frame flags.
+  // @param length supplies the GRPC data frame length.
+  // @param output the buffer to store the encoded data, it's size must be 5.
+  void NewFrame(uint8_t flags, uint64_t length, uint8_t* output);


using std::array<uint8_t, 5>? (would be good to enforce size 5 requirement here)

RomanDzhabarov · 2017-02-08T06:34:34Z

source/common/grpc/codec.h

+  bool Decode(Buffer::Instance& input, std::vector<Frame>* output);
+
+private:
+  // Wiring format of GRPC data frame header:


can you also put a link to official docs if any for this format?

RomanDzhabarov · 2017-02-08T06:38:46Z

source/common/grpc/codec.h

+  // @param flags supplies the GRPC data frame flags.
+  // @param length supplies the GRPC data frame length.
+  // @param output the buffer to store the encoded data, it's size must be 5.
+  void NewFrame(uint8_t flags, uint64_t length, uint8_t* output);


RomanDzhabarov · 2017-02-08T06:38:58Z

source/common/grpc/codec.h

+  // @param input supplies the binary octets wrapped in a GRPC data frame.
+  // @param input supplies the buffer to store the decoded data.
+  // @return bool whether the decoding success.
+  bool Decode(Buffer::Instance& input, std::vector<Frame>* output);


RomanDzhabarov · 2017-02-08T06:44:49Z

source/common/grpc/codec.h

+struct Frame {
+  uint8_t flags;
+  uint32_t length;
+  Buffer::Instance* data;


Buffer::InstancePtr

Change to use Buffer::IntancePtr.

mattklein123

a few nits otherwise looks good

mattklein123 · 2017-02-09T00:22:59Z

source/common/grpc/codec.h

@@ -0,0 +1,78 @@
+#pragma once
+
+#include <array>


del or add to precompiled header

mattklein123 · 2017-02-09T00:24:10Z

source/common/grpc/codec.h

+enum class CompressionAlgorithm { None, Gzip };
+
+struct Frame {
+  uint8_t flags;


_ postfix for members (.e.g, flags_)

Is it needed for public struct?

We have been doing this for public structs so to be consistent I would change it. (I don't really care that much about this but might as well be consistent).

mattklein123 · 2017-02-09T00:24:46Z

source/common/grpc/codec.h

+  // Creates a new GRPC data frame with the given flags and length.
+  // @param flags supplies the GRPC data frame flags.
+  // @param length supplies the GRPC data frame length.
+  // @param output the buffer to store the encoded data, it's size must be 5.


"... data. Its size must be 5."

mattklein123 · 2017-02-09T00:25:49Z

source/common/grpc/codec.h

+  // Decodes the given buffer with GRPC data frame.
+  // @param input supplies the binary octets wrapped in a GRPC data frame.
+  // @param output supplies the buffer to store the decoded data.
+  // @return bool whether the decoding success.


"... succeeded or not."

mattklein123 · 2017-02-09T00:26:01Z

source/common/grpc/codec.h

+  bool decode(Buffer::Instance& input, std::vector<Frame>& output);
+
+private:
+  // Wiring format (http://www.grpc.io/docs/guides/wire.html) of GRPC data frame


"Wire format ..."

mattklein123 · 2017-02-09T00:26:37Z

source/common/grpc/codec.h

+  //
+  // A fixed header consists of five bytes.
+  // The first byte is the Flag. The last one "C" bit indicates if the message
+  // is compressed or not (0 is uncompressed, 1 is compressed). The rest seven


The other seven

mattklein123 · 2017-02-09T00:26:44Z

source/common/grpc/codec.h

+  // A fixed header consists of five bytes.
+  // The first byte is the Flag. The last one "C" bit indicates if the message
+  // is compressed or not (0 is uncompressed, 1 is compressed). The rest seven
+  // "R" bits is reserved for future use.


are reserved

mattklein123 · 2017-02-09T00:27:14Z

test/common/grpc/codec_test.cc

@@ -0,0 +1,115 @@
+#include <array>


del or add to precompiled header

mattklein123 · 2017-02-09T00:27:23Z

test/common/grpc/codec_test.cc

+
+#include "common/buffer/buffer_impl.h"
+#include "common/grpc/codec.h"
+#include "test/generated/helloworld.pb.h"


nit: newline before this line

mattklein123 · 2017-02-09T00:27:43Z

test/common/grpc/codec_test.cc

+    EXPECT_EQ("hello", result.name());
+  }
+}
+} // Grpc


nit: newline before this line

RomanDzhabarov · 2017-02-09T00:53:49Z

source/common/grpc/codec.cc

+          output.push_back(std::move(frame_));
+          frame_.flags = 0;
+          frame_.length = 0;
+          frame_.data.reset();


do not think there is a need to .reset() here as ownership was transferred to the frame in output

Right, done.

RomanDzhabarov · 2017-02-09T01:19:24Z

test/common/grpc/codec_test.cc

+
+  std::vector<Frame> frames;
+  Decoder decoder;
+  decoder.decode(buffer, frames);


EXPECT_TRUE and in the case below

RomanDzhabarov · 2017-02-09T01:19:42Z

LGTM, one comment left

lizan · 2017-02-09T03:17:22Z

source/common/grpc/codec.cc

+      }
+    }
+  }
+  return true;


We might want to check state_ to see if the input was a partial GRPC frame? (e.g. frame length = 100 but only 10 bytes in input buffer.
Also add a test for that case.

Test added.
You don't need to check the state_. If grpc call get aborted when decoding, you should be able to know that from the HTTP2 layer, as the stream will be reset. Any intermediate state and buffer can be discarded. The workflow would be keep feeding data to the decoder as they arrive and check if there's one or more complete frame get decoded.

mattklein123 · 2017-02-09T15:58:32Z

@fengli79 one other thing came to mind when I read @lizan comment. You aren't draining the buffer anywhere. I'm not sure if this is intentional, but either way I think some changes are needed:

If you intend for the caller to drain the buffer, you should change decode() to take a const Buffer::Instance&, and document that decode consumes all bytes if it returns true. Then the caller can drain.
Have decode() drain the buffer itself, and document that it will drain all data, and add tests to this effect.

fengli79 · 2017-02-09T16:13:17Z

It's not intended, the caller need to drain the data when needed, as the test did.

…

On Thu, Feb 9, 2017 at 7:58 AM Matt Klein ***@***.***> wrote: @fengli79 <https://github.com/fengli79> one other thing came to mind when I read @lizan <https://github.com/lizan> comment. You aren't draining the buffer anywhere. I'm not sure if this is intentional, but either way I think some changes are needed: 1. If you intend for the caller to drain the buffer, you should change decode() to take a const Buffer::Instance&, and document that decode consumes all bytes if it returns true. Then the caller can drain. 2. Have decode() drain the buffer itself, and document that it will drain all data, and add tests to this effect. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT1vm4lDTp4F8x10Sp3-DqHw_2g1crNvks5raze2gaJpZM4L1tyn> .

fengli79 · 2017-02-09T16:19:12Z

The decoder takes all the data, no matter it returns true or false. It can be written in a way to drain the data always to make it work like a move. However let the caller drain data as needed is more flexible. The envoy buffer is a wrapper which is not very convenient for decoding, to get best performance, I would prefer to get a buffer can be modified in flight, like remove/modify byte in a slice and add/remove slice in the buffer.

…

On Thu, Feb 9, 2017 at 8:13 AM Feng Li ***@***.***> wrote: It's not intended, the caller need to drain the data when needed, as the test did. On Thu, Feb 9, 2017 at 7:58 AM Matt Klein ***@***.***> wrote: @fengli79 <https://github.com/fengli79> one other thing came to mind when I read @lizan <https://github.com/lizan> comment. You aren't draining the buffer anywhere. I'm not sure if this is intentional, but either way I think some changes are needed: 1. If you intend for the caller to drain the buffer, you should change decode() to take a const Buffer::Instance&, and document that decode consumes all bytes if it returns true. Then the caller can drain. 2. Have decode() drain the buffer itself, and document that it will drain all data, and add tests to this effect. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT1vm4lDTp4F8x10Sp3-DqHw_2g1crNvks5raze2gaJpZM4L1tyn> .

mattklein123 · 2017-02-09T16:33:14Z

OK that's fine. Then just make the param const and document that all bytes are consumed if it's not already documented.

Change the CodecTest to GrpcCodecTest.

fengli79 · 2017-02-09T18:04:41Z

@mattklein123 With a second think, I change it to drain the inputs when decoding succeeded. Also change the CodecTest to GrpcCodecTest to avoid naming conflicts with other tests.

mattklein123 · 2017-02-09T18:09:27Z

@fengli79 looks good. I forgot that you need to sign CLA. Can you ACK back when done and I will merge.

fengli79 · 2017-02-09T18:22:40Z

Signed.

…

On Thu, Feb 9, 2017 at 10:09 AM Matt Klein ***@***.***> wrote: @fengli79 <https://github.com/fengli79> looks good. I forgot that you need to sign CLA. Can you ACK back when done and I will merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#414 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AT1vm9uUoeSe4xkW9wGMfnr1W945PgGQks5ra1ZagaJpZM4L1tyn> .

* Revert "envoy sha update (envoyproxy#414)" This reverts commit ebb5ef1. * Revert "Fix mixer forward attribute bug. (envoyproxy#412)" This reverts commit 4f07917.

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of mixerclient This PR will be merged automatically once checks are successful. ```release-note none ```

….rst (envoyproxy#414) * zh-translation:docs/root/configuration/observability/access_log/usage.rst * zh-translation:docs/root/configuration/observability/access_log/usage.rst * zh-translation:docs/root/configuration/observability/access_log/usage.rst * zh-translation:docs/root/configuration/observability/access_log/usage.rst Co-authored-by: 包仁义 <renyi.bao@yiducloud.cn>

**Commit Message** This fixes the latest chart's image tag which previously was set to v0.0.0-latest. This fixes the helm-package recipe and now it points to :latest. **Related Issues/PRs (if applicable)** Follow up on #395 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

fengli79 added 5 commits February 2, 2017 14:02

Add codec to encode/decode GRPC data frame.

4f52eb5

Revert "Add codec to encode/decode GRPC data frame."

02c7880

This reverts commit 4f52eb5.

Add codec to encode/decode GRPC data frame.

bf3037c

Update codec.cc

d2e7195

Update codec_test.cc

9f4babe

fengli79 added 3 commits February 3, 2017 11:06

Fix the format.

b5a5019

Change ByteSizeLong() to ByteSize().

62c7b05

Fix memory leak in tests.

549a9b4

RomanDzhabarov reviewed Feb 8, 2017

View reviewed changes

Rename methods to follow envoy naming convention.

e829410

Change to use Buffer::IntancePtr.

mattklein123 reviewed Feb 9, 2017

View reviewed changes

Address comments.

cd47044

RomanDzhabarov reviewed Feb 9, 2017

View reviewed changes

fengli79 added 2 commits February 8, 2017 16:59

Remove unnecessary reset().

8965a8b

Add _ surfix for struct members.

7d4bac2

mattklein123 approved these changes Feb 9, 2017

View reviewed changes

RomanDzhabarov reviewed Feb 9, 2017

View reviewed changes

Add EXPECT_TRUE to verify decode() is succeeded.

876edf1

lizan reviewed Feb 9, 2017

View reviewed changes

fengli79 added 2 commits February 8, 2017 20:21

Add test for incomplete frame decoding.

281f781

Add test for incomplete frame decoding.

595abc6

Drains the input buffer when decoding succeeded.

fadd7a4

Change the CodecTest to GrpcCodecTest.

mattklein123 merged commit f44e437 into envoyproxy:master Feb 9, 2017

rshriram added a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

envoy sha update (envoyproxy#414)

ebb5ef1

Conversation

fengli79 commented Feb 2, 2017

Uh oh!

PiotrSikora commented Feb 3, 2017

Uh oh!

mattklein123 commented Feb 3, 2017

Uh oh!

louiscryan commented Feb 3, 2017

Uh oh!

fengli79 commented Feb 8, 2017

Uh oh!

mattklein123 commented Feb 8, 2017

Uh oh!

RomanDzhabarov commented Feb 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!